Skip to content

chore(ALL): fix packaging #316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Dec 16, 2021
Merged

chore(ALL): fix packaging #316

merged 7 commits into from
Dec 16, 2021

Conversation

flochaz
Copy link
Contributor

@flochaz flochaz commented Dec 15, 2021 •
edited
Loading

Description of your changes

Current dependency tree makes it hard to package cleanly.

After isolating LambdaInterface in a dedicated shared package (#314) we can now rely on this package and have a clean npm.

Some details on the changes:

  • we need a dedicated tsconfig-dev.json to make sure all folders get linted but only src get packaged.
  • We need to move types into src to avoid to create 2 levels in tgz (lib/src/index.js and lib/type/index.js but lib/index.js)
  • we need to ignore /types/ in coverage because it's not recognized as covered by jest.

How to verify this change

npm login

➜  logger git:(fix/packaging) ✗ rm -rf lib && npm run build && npm run package
> @aws-lambda-powertools/logger@0.0.0 build
> tsc


> @aws-lambda-powertools/logger@0.0.0 package
> mkdir -p dist/ && npm pack && mv *.tgz dist/

npm notice 
npm notice 📦  @aws-lambda-powertools/logger@0.0.0
npm notice === Tarball Contents === 
npm notice 14.2kB  README.md                                      
npm notice 585B    lib/config/ConfigService.d.ts                  
npm notice 540B    lib/config/ConfigService.d.ts.map              
npm notice 932B    lib/config/ConfigService.js                    
npm notice 296B    lib/config/ConfigServiceInterface.d.ts         
npm notice 367B    lib/config/ConfigServiceInterface.d.ts.map     
npm notice 323B    lib/config/ConfigServiceInterface.js           
npm notice 700B    lib/config/EnvironmentVariablesService.d.ts    
npm notice 657B    lib/config/EnvironmentVariablesService.d.ts.map
npm notice 3.5kB   lib/config/EnvironmentVariablesService.js      
npm notice 157B    lib/config/index.d.ts                          
npm notice 181B    lib/config/index.d.ts.map                      
npm notice 1.0kB   lib/config/index.js                            
npm notice 149B    lib/formatter/index.d.ts                       
npm notice 184B    lib/formatter/index.d.ts.map                   
npm notice 1.0kB   lib/formatter/index.js                         
npm notice 461B    lib/formatter/LogFormatter.d.ts                
npm notice 474B    lib/formatter/LogFormatter.d.ts.map            
npm notice 2.2kB   lib/formatter/LogFormatter.js                  
npm notice 305B    lib/formatter/LogFormatterInterface.d.ts       
npm notice 353B    lib/formatter/LogFormatterInterface.d.ts.map   
npm notice 323B    lib/formatter/LogFormatterInterface.js         
npm notice 351B    lib/formatter/PowertoolLogFormatter.d.ts       
npm notice 387B    lib/formatter/PowertoolLogFormatter.d.ts.map   
npm notice 1.9kB   lib/formatter/PowertoolLogFormatter.js         
npm notice 197B    lib/helpers.d.ts                               
npm notice 266B    lib/helpers.d.ts.map                           
npm notice 571B    lib/helpers.js                                 
npm notice 88B     lib/index.d.ts                                 
npm notice 144B    lib/index.d.ts.map                             
npm notice 912B    lib/index.js                                   
npm notice 98B     lib/log/index.d.ts                             
npm notice 153B    lib/log/index.d.ts.map                         
npm notice 934B    lib/log/index.js                               
npm notice 569B    lib/log/LogItem.d.ts                           
npm notice 527B    lib/log/LogItem.d.ts.map                       
npm notice 2.4kB   lib/log/LogItem.js                             
npm notice 241B    lib/log/LogItemInterface.d.ts                  
npm notice 301B    lib/log/LogItemInterface.d.ts.map              
npm notice 303B    lib/log/LogItemInterface.js                    
npm notice 2.0kB   lib/Logger.d.ts                                
npm notice 1.7kB   lib/Logger.d.ts.map                            
npm notice 17.8kB  lib/Logger.js                                  
npm notice 67B     lib/types/formats/index.d.ts                   
npm notice 145B    lib/types/formats/index.d.ts.map               
npm notice 885B    lib/types/formats/index.js                     
npm notice 2.1kB   lib/types/formats/PowertoolLog.d.ts            
npm notice 756B    lib/types/formats/PowertoolLog.d.ts.map        
npm notice 311B    lib/types/formats/PowertoolLog.js              
npm notice 84B     lib/types/index.d.ts                           
npm notice 153B    lib/types/index.d.ts.map                       
npm notice 920B    lib/types/index.js                             
npm notice 841B    lib/types/Log.d.ts                             
npm notice 821B    lib/types/Log.d.ts.map                         
npm notice 271B    lib/types/Log.js                               
npm notice 2.0kB   lib/types/Logger.d.ts                          
npm notice 1.9kB   lib/types/Logger.d.ts.map                      
npm notice 279B    lib/types/Logger.js                            
npm notice 404.6kB npm-shrinkwrap.json                            
npm notice 2.8kB   package.json                                   
npm notice === Tarball Details === 
npm notice name:          @aws-lambda-powertools/logger           
npm notice version:       0.0.0                                   
npm notice filename:      @aws-lambda-powertools/logger-0.0.0.tgz 
npm notice package size:  119.8 kB                                
npm notice unpacked size: 479.7 kB                                
npm notice shasum:        c76a8d99f53591f3904a8b7c3ff14fc332d8e010
npm notice integrity:     sha512-+IcQDjb5t7Nau[...]K1HFNP6MRSKww==
npm notice total files:   60                                      
npm notice 
aws-lambda-powertools-logger-0.0.0.tgz

Related issues, RFCs

#136

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published in downstream module
  • The PR title follows the conventional commit semantics

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@flochaz flochaz changed the title fix linting chore(logger): fix packaging Dec 15, 2021
@dreamorosi dreamorosi self-requested a review December 15, 2021 12:41
@dreamorosi dreamorosi added this to the beta-release milestone Dec 15, 2021
dreamorosi
dreamorosi previously approved these changes Dec 15, 2021
View reviewed changes
@flochaz flochaz mentioned this pull request Dec 15, 2021
Merged
10 tasks
@flochaz flochaz changed the title chore(logger): fix packaging chore(ALL): fix packaging Dec 15, 2021
saragerion
saragerion reviewed Dec 16, 2021
View reviewed changes
packages/logger/.eslintrc.json
@@ -12,7 +12,7 @@
"parserOptions": {
"ecmaVersion": 2020,
"sourceType": "module",
"project": "./tsconfig.json"
"project": "./tsconfig-dev.json"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not disagreeing, just curious: why do we need a tsconfig-dev vs prod?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explained it in pr details but basically we need one for eslint and for packaging to get everything lint but just src packaged

saragerion
saragerion reviewed Dec 16, 2021
View reviewed changes
Copy link
Contributor

@saragerion saragerion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great but I am concerned that moving folders around now will overwrite some of the changes that are being done in other PRs and already merged from other packages

@flochaz
Copy link
Contributor Author

flochaz commented Dec 16, 2021

This looks great but I am concerned that moving folders around now will overwrite some of the changes that are being done in other PRs and already merged from other packages

it's not compulsory to move types but it makes final package a lot cleaner from my point of view. if it's too much of a refacto I can change it back but I'll need a bit of time tomorrow to do so

@flochaz flochaz merged commit 38c5103 into main Dec 16, 2021
@flochaz flochaz deleted the fix/packaging branch December 16, 2021 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saragerion saragerion saragerion left review comments

@dreamorosi dreamorosi dreamorosi left review comments

Assignees

@flochaz flochaz

Labels
None yet
Projects
None yet
Milestone
beta-release
Development

Successfully merging this pull request may close these issues.
3 participants
@flochaz @dreamorosi @saragerion